fix(0.6.0): establish explicit runtime lifecycle ownership - #226

Round-5 rework landed — all P1/P2/P3 items addressed (review this at head e4cdcae5+, not 5feef47f)

Your review was against 5feef47f; round-5 (e4cdcae5, pushed before this comment) implements the recommended root fix. Per-point:

P1-1A — Blocking calls aren't tracked

Now runBlocking(lifecycleJob + engineThreadMarker.asContextElement(true)) { execute(...) } — the blocking call is a child of the engine's internal lifecycleJob. close() cancels lifecycleJob and joins it, so a blocking provider in a 30s suspension is cancelled and its NonCancellable cleanup runs before close() returns.
Regression: blocking invocation in long suspension is cancelled and joined by close (provider enters awaitCancellation(), close() from another thread, asserts cleanup completed within close()).

P1-1B — Streaming collections aren't owned

Provider collection now runs in lifecycleScope.launch (a child of lifecycleJob), chunks bridged to the collector's emit via an UNLIMITED Channel (emit stays in the collector's coroutine — SafeCollector invariant). close() cancels lifecycleJob → the collection job is cancelled → provider finally executes → close() joins lifecycleJob and waits. The per-chunk emitWhileOpen gate is retained as belt-and-suspenders; the TOCTOU you flagged is closed by ownership, not by the boolean check.
Regressions: streaming collection suspended indefinitely is cancelled and cleaned up by close (provider emits first, awaitCancellation() forever, close() → cleanup completes within close), plus the existing mid-collection close terminates an in-flight stream.

P1-1C — Tracked suspend calls cancelled but not joined

close() now joins every tracked invocation: runBlocking { lifecycleJob.join(); tracked.forEach { it.join() } } — cancellation is a request; NonCancellable cleanup must complete before close() returns. The self-close marker (engineThreadMarker) still skips the join when close() is called from an engine-owned coroutine (can't join your own job).

P1-2 — Same-job deadlock with public constructor API

Fixed by the ownership distinction, not ThreadLocal: close() touches only lifecycleJob + tracked jobs — the caller-supplied job/scope ctor params are never cancelled or joined (they remain for ABI). TramaiEngine(provider, job = coroutineContext.job, scope = this); close() no longer deadlocks.
Regression: close does not deadlock when caller supplied its own job and scope.

P2-1 — Vacuous external-provider test

Now forces tramai.runtime() before close so the assertion runs against a real engine.

P3 — Roadmap

docs/ROADMAP-0.6.0.md marks Epic 1.3 ✅ Complete — PR #226 at line 359, matching Epics 1.1/1.2.

Verification: full ./gradlew test --rerun-tasks green (2m45s), verifyPr PASSED (41 files), maintainability baseline PASSED, verifyCancellationSafety PASSED, apiCheck additive (AutoCloseable only). Suspend invocations run on the engine's own dispatcher (caller Job retained via minusKey(ContinuationInterceptor) only) so the parent-cancellation contract tests still pass.

Requesting re-review at the new head.

Round-6 rework landed — P1/P2/P3 resolved at head af5e1a4a

All four round-6 findings addressed:

P1 — stream-start vs close race can hang the caller

Channel termination now depends on job completion, not on the collection body having started: collectJob.invokeOnCompletion { cause -> chunks.close(cause) } is registered immediately after the launch. If close() cancels lifecycleJob after the flow's open check but before the launched body runs, the invokeOnCompletion still fires and closes the channel — the collector terminates instead of hanging forever on receive. (Same pre-start protection pattern you already approved for suspend invocations.)
Regression: stream start racing close never hangs the collector — 200 iterations of the exact admission race, each bounded by withTimeout(5s).

P2 — Channel.UNLIMITED destroys backpressure

Switched to Channel.RENDEZVOUS. A slow collector now blocks the provider's next send instead of letting it buffer unboundedly; take(1) and slow-collector semantics match a direct flow connection.
Regression: streaming bridge preserves backpressure when the collector is slow — collector blocks after chunk 1, asserts the provider emitted exactly 1 chunk (UNLIMITED would have let it race ahead).

P2 — lifecycle handler could log raw user-supplied exceptions

Two-part fix: (1) streaming failures are now captured into collectFailure and surfaced to the collector through the channel drain — the collection job no longer rethrows, so arbitrary throwables (e.g. an OperationInterceptor throwing RuntimeException("customer-token=SECRET")) never reach the CoroutineExceptionHandler; (2) the handler itself logs fixed safe metadata only — the exception type's qualified name — never the raw throwable, consistent with Epic 1.2 safe-error-boundary work.

P3 — stale dispatcher comment

close()'s comment now states the implementation's actual behavior: invocation jobs run on the engine's own dispatcher (caller ContinuationInterceptor stripped at launch), so joining is safe as long as close() isn't called from a coroutine dispatched on that same engine dispatcher.

Verification: full ./gradlew test --rerun-tasks green (2m33s, 212 tasks), TramaiEngineTest green including both new regressions, apiCheck additive. Requesting re-review.

Round-7 rework landed — P1/P2/P3 addressed at head 3f1c4f8a

P1 — streaming self-close deadlock (merge blocker)

The engine-thread marker now lives on lifecycleScope itself: CoroutineScope(lifecycleJob + Dispatchers.Default + engineThreadMarker.asContextElement(true) + exceptionHandler). Every engine-owned child — including the streaming collection job — inherits the self-close protection automatically, so engine.close() called from inside a streaming provider/interceptor/observer skips the join and cannot self-deadlock. This encodes ownership once at the scope level exactly as you recommended (stronger than decorating individual launches, and it covers future lifecycle tasks for free). The blocking path keeps its explicit marker (runBlocking isn't scope-launched); the suspend launch's explicit marker is now redundant but harmless.

Regression added, exactly as specified: self close from streaming owned coroutine does not deadlock — provider flow calls engine.close() mid-collection; withTimeout(2_000) proves termination.

P2 — roadmap leak-test evidence matrix

Epic 1.3 now documents the matrix mapping all five roadmap task-6 requirements to concrete tests:

P3 — timing-based tests

The new lifecycle regressions use withTimeout bounds and CompletableDeferred gates, not scheduler-time assumptions; the one delay(300) (backpressure test) is a deliberate negative-observation window for a broken UNLIMITED bridge, not a correctness gate.

Verification: full ./gradlew test verifyPr --rerun-tasks green (3m, 212 tasks), apiCheck additive, engine suite green including the new regression. Requesting re-review at the new head.

Commit-location clarification + round-8 cleanup

Head reconciliation: the round-7 commit is 9c0a736c — already on this branch and the PR head (your gh check confirms identical/ahead_by 0). My previous comment's "at head 3f1c4f8a" was a typo in the comment body, not a missing push. The scope-level engine-thread marker (9c0a736c) is what you reviewed. No phantom push — sorry for the hunt.

Round-8 items, all addressed at the new head:

P2 — shutdown-hook leak proof

New regression in tramai-orchestration TramaiWorkerTest: close deregisters the JVM shutdown hook and retains no reference — reflects the private shutdownHook field: non-null after start() (hook registered), null after close() (deregistered and no reference retained).

P3 — HTTP-stream evidence row

Roadmap now cites the actual provider-level InputStream cleanup tests in tramai-openai OpenAiProviderTest: stream closes response body after done marker, stream closes response body after malformed chunk, stream closes response body when collector stops after first token, mid stream io failure is retryable sanitized and observed — plus the #226 engine bridge tests and the springboot example E2E.

P3 — timing synchronization

• The suspended-stream cleanup test's delay(200) admission assumption is now a deterministic firstChunkDelivered CompletableDeferred gate.
• The backpressure test's delay(300) is gone: it now proves backpressure structurally — the provider ATTEMPTS chunk 2 then STALLS at the rendezvous send (attempted == 2, delivered == 1), no sleep involved.

Verification: engine + orchestration suites green (incl. new tests), full test verifyPr running. Requesting re-review at the new head.

Round-8 follow-up landed — shutdown-hook deregistration now mutation-proven (head 4c1d9a3e)

You were right: the previous regression proved reference clearing, not JVM deregistration. Fixed exactly as you specified.

Strengthened regression (TramaiWorkerTest)

After worker.close():

• hookField.get(worker) is null (no retained Thread reference) ✅
• Runtime.getRuntime().removeShutdownHook(requireNotNull(hookAfterStart)) returns false — the hook is already absent from the JVM registry ✅
• worker.close() wrapped in try/finally: an earlier assertion failure can never leave a hook registered in the test JVM

Mutation-sensitivity verified

I actually performed the mutation you described (commented out Runtime.getRuntime().removeShutdownHook(hook) in TramaiWorker.shutdown, kept shutdownHook = null): the strengthened regression FAILED as designed (BUILD FAILED). Restored the production call — the test passes again. So the regression now fails when the real deregistration statement disappears, which is exactly the leak Task 6 must catch.

Roadmap row corrected

The shutdown-hooks evidence row now cites this test first: close deregisters the JVM shutdown hook and retains no reference — proves the registered hook is absent from Runtime after close (removeShutdownHook returns false) and the worker retains no Thread reference — plus the Spring destroyMethod/context-shutdown and standalone idempotency tests as secondary evidence.

Verification: orchestration suite green (46s), full test verifyPr green (3m9s, EXIT 0), production TramaiWorker.kt unchanged in this commit (test + roadmap only). CI re-running on the new head.

Also noting for the record: the earlier "3f1c4f8a" was a typo in my comment body — the round-7 commit has always been 9c0a736c, on this branch.

Requesting re-review — expecting this to be the close-out.